-
-
Notifications
You must be signed in to change notification settings - Fork 97
1724 - Add MUI Modal open/close behavior #2030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1724 - Add MUI Modal open/close behavior #2030
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR converts the existing modal implementation to use Material-UI's Modal component with proper open/close behavior. The changes enable users to close the modal via the Escape key, Close button, or by clicking outside the modal box.
Key Changes:
- Replaced the basic
<div>modal structure with MUI'sModalandBoxcomponents - Added state management for modal open/close behavior using
useEffectanduseState - Integrated
handleClosefunction to properly clean up state when the modal is closed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you for the review! I've made the updates and resolved the conversation. |
|
@bconti123 approved, good to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for your patience while I reviewed this.
Question (non-blocking): Was there a reason you removed ARIA attributes?
Suggestion (non-blocking): You can simplify this component by removing both the useEffect and the open state. Currently, the open state syncs with the selectedEvent state, so we’re maintaining two states that represent the same piece of information. Instead of duplicating state, we can derive it. useEffect is also not necessary, because React will automatically re-render when selectedEvent changes.
Current approach:
const [open, setOpen] = useState(false);
useEffect(() => {
if (selectedEvent) { setOpen(true); }
else { setOpen(false); }
}, [selectedEvent]);Suggested approach (derived state):
const open = Boolean(selectedEvent);open is now derived from selectedEvent and the useEffect can be removed entirely.
Your modal would stay the same:
<Modal open={open} onClose={handleClose}>To close it, you only need to reset selectedEvent, so handleClose() would just be:
function handleClose() {
setFormErrors(null);
setSelectedEvent(null);
}When selectedEvent becomes null, the component re-renders, open becomes false, and the modal closes. This avoids duplicated state and follows React best practices around deriving state instead of duplicating state.
|
@eunicode Thank you for your feedback. I like the suggested approach. This makes sense. You are right that the Regarding your question about the ARIA attributes, yes, there was a reason in the reviewed changes, and that conversation is now resolved. I did not find any IDs that matched the ARIA attributes, so I resolved the thread. I will merge this now. |
Fixes #1724
What changes did you make and why did you make them ?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied